Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/console virtualization #224

Merged
merged 32 commits into from
Mar 5, 2024
Merged

Conversation

johann-crabnebula
Copy link
Contributor

@johann-crabnebula johann-crabnebula commented Feb 7, 2024

TL;DR

This PR aims to improve the performance of the console tab. The goal was to at least be able to confidently display 10k logs and make the experience as smooth as possible. The dependency tanstack-virtual for solidjs was added to make sure we only render the DOM elements which are necessary for the view.

Acceptance criteria

  • General handling of 10k + logs
  • Switching between tabs without breaking (for example from console -> sources -> console / note that the calls tab is currently not working with a large number of spans, so please try to test the other tabs when switching, PR for the calls tab is coming)
  • Filtering should be smooth with logs coming in and when inactive
  • Console tab should work fine with cached logs when disconnected

Let me know

If there is anything odd in general please let me know.

Help with testing this

In my testing I just spammed events from my tauri app. Please feel free to be more creative with testing:

let mut count = 0;
// Infinite loop
loop {
    count += 1;
    tokio::time::sleep(Duration::from_millis(1)).await;
    window
        .emit(
            format!("test{}-event", count).as_str(),
            &EventPayload {
                key: "count",
                value: count.to_string(),
            },      
    ).unwrap();
    tracing::debug!("test{}-event", count);

    if count == 15000 {
        // Exit this loop
        break;
    }
}

Fixes DT-112

Copy link

netlify bot commented Feb 7, 2024

Deploy Preview for cn-devtools-app ready!

Name Link
🔨 Latest commit a2df0f5
🔍 Latest deploy log https://app.netlify.com/sites/cn-devtools-app/deploys/65e5fb6ac8bde1000810f653
😎 Deploy Preview https://deploy-preview-224--cn-devtools-app.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@johann-crabnebula johann-crabnebula force-pushed the feat/console-virtualization branch from c9b8e6a to 54c559a Compare February 7, 2024 10:22
@johann-crabnebula johann-crabnebula force-pushed the feat/console-virtualization branch from e0bedd9 to 529361e Compare February 8, 2024 03:55
@CrabNejonas
Copy link
Contributor

quick tip @johann-crabnebula in Rust you can do this to loop through a range of number:

    for i in 0..15000 {
        tokio::time::sleep(Duration::from_millis(1)).await;
        window
            .emit(
                format!("test{}-event", count).as_str(),
                &EventPayload {
                    key: "count",
                    value: count.to_string(),
                },
            )
            .unwrap();
        tracing::debug!("test{}-event", count);
    }

CrabNejonas
CrabNejonas previously approved these changes Feb 8, 2024
Copy link
Contributor

@CrabNejonas CrabNejonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, not gonna argue with you about any JS stuff, but perfand usability is good 👍

CrabNejonas
CrabNejonas previously approved these changes Feb 8, 2024
Copy link
Contributor

@denjell-crabnebula denjell-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johann and I talked this through, and it seems good enough for now. I think we should ship it and then focus on speed / perf enhancements for the Tauri app version.

Copy link
Contributor

@atila-crabnebula atila-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few questions, so I'll wait for Johann to be back before merging this.

Comment on lines 65 to 68
<Show when={metadata?.location?.file}>
{(filePath) => (
<>
{getFileNameFromPath(filePath())}:{metadata?.location?.line ?? ""}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be better to handle the non-existing line within getFileNameFromPath and omitting the DOM node completely instead of printing an empty string. So the method returns the line or a falsy value (null, perhaps).

So the signature looks like:

<Show when={getFileNameFromPath(metadata?.location?.file)>
  {line => (<span>{line()</span>)}
</Show>

wdyt?

Copy link
Contributor Author

@johann-crabnebula johann-crabnebula Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this, taking your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote a getFileLineFunction that either returns a fully fledged line or null:

import { Location } from "../proto/common";
import { getFileNameFromPath } from "./get-file-name-from-path";

export function getFileLineFromLocation(location: Location | undefined) {
  if (!location || !location.file) return null;

  let line = getFileNameFromPath(location.file);

  if (location.line) line += `:${location.line}`;

  return line;
}

Template now looks like this:

<Show when={getFileLineFromLocation(metadata?.location)}>
  {(line) => <span>{line()}</span>}
</Show>

)
);
setMonitorData("spans", (spans) => [
...updatedSpans(spans, spansUpdate.spanEvents),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mutating the entire array. Every time this setMonitorData is called, we replace the entire array. How about wrapping it in a reconcile() so only the needed keys are mutated? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to use Solid's reconcile here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the use of reconcile the updatedSpans function and the receiving piece on the calls tab might need updates. I would like to postpone this change to the calls virtualization PR.

…zation

# Conflicts:
#	clients/web/package.json
#	clients/web/pnpm-lock.yaml
#	clients/web/src/components/autoscroll-pane.tsx
#	clients/web/src/lib/connection/transport.tsx
#	clients/web/src/lib/console/filter-logs.ts
#	clients/web/src/views/dashboard/console.tsx
@atila-crabnebula atila-crabnebula self-requested a review February 21, 2024 13:13
Copy link
Contributor

@atila-crabnebula atila-crabnebula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is going on at the spans tab.

Bug

  1. Run tauri-v1 example
  2. Open the spans tab
  3. Fire a span test

It will never catch the end event.

@johann-crabnebula
Copy link
Contributor Author

Reverted the change for the spans. It will slow down the overall performance but this unblocks the PR, so we can move on.

@johann-crabnebula johann-crabnebula merged commit 068f98f into main Mar 5, 2024
6 checks passed
@johann-crabnebula johann-crabnebula deleted the feat/console-virtualization branch March 5, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants